-
Notifications
You must be signed in to change notification settings - Fork 1
feat: 경매장 거래내역 적재 다음날 새벽 통계 집계 스케쥴러 추가 #85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ 테스트 결과 for PRBuild: success 🧪 테스트 실행 with Gradle |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a comprehensive statistics aggregation system to replace the previous simple min-price tracking. It introduces daily and weekly statistics calculated across three levels (item, subcategory, and top category), with schedulers that run after auction history data is loaded. The implementation includes database migrations, JPA entities, repositories with native SQL queries, service layers, REST controllers, and corresponding tests.
Key Changes:
- Adds daily statistics scheduler triggered by auction history save events
- Adds weekly statistics scheduler (runs Mondays at 4 AM) and previous-day statistics finalization scheduler (runs daily at 00:10)
- Replaces old ItemDailyMinPrice functionality with comprehensive multi-level statistics
Reviewed changes
Copilot reviewed 68 out of 73 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| V13__create_statistics_tables.sql | Creates 6 new statistics tables (3 daily, 3 weekly) for item/subcategory/topcategory levels |
| application.yml | Adds statistics.previous-day.cron configuration property |
| DailyStatisticsService.java | Service handling current-day and previous-day statistics calculation |
| PreviousDayStatisticsScheduler.java | Scheduler for finalizing previous day statistics at 00:10 |
| WeeklyStatisticsScheduler.java | Scheduler for weekly statistics aggregation on Mondays |
| *Repository.java | Native SQL queries for upsert operations on statistics tables |
| AuctionHistorySavedEvent.java | Event published after auction history save to trigger statistics update |
| OpenApiAuctionHistoryMapper.java | Changed to convert UTC timestamps to KST by adding 9 hours |
| PageRequestDto.java | Modified page calculation logic |
| docker-compose files | Updated database configuration and removed min-price cron |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| FROM item_weekly_statistics iws | ||
| INNER JOIN auction_history ah ON iws.item_name = ah.item_name |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to TopCategoryWeeklyStatisticsRepository, joining with auction_history to get category mappings may result in incorrect or duplicate data. The same item_name can appear in auction_history with different sub_categories. Consider using a more reliable source for category relationships.
| default Instant utcToKst(Instant utcTime) { | ||
| // API에서 받은 UTC 시간에 9시간을 더하여 KST로 변환 | ||
| return utcTime != null ? utcTime.plus(9, ChronoUnit.HOURS) : null; |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mapper is converting UTC time to KST by adding 9 hours, but this assumes the API always returns UTC. The variable name and comment should clarify this assumption. Additionally, verify that the API actually returns UTC timestamps as this conversion could lead to incorrect data if the assumption is wrong.
| default Instant utcToKst(Instant utcTime) { | |
| // API에서 받은 UTC 시간에 9시간을 더하여 KST로 변환 | |
| return utcTime != null ? utcTime.plus(9, ChronoUnit.HOURS) : null; | |
| default Instant utcToKst(Instant apiUtcInstant) { | |
| // 외부 OpenAPI가 UTC 타임스탬프를 반환한다고 가정하고, KST(UTC+9)로 변환한다. | |
| return apiUtcInstant != null ? apiUtcInstant.plus(9, ChronoUnit.HOURS) : null; |
| FROM subcategory_daily_statistics sds | ||
| INNER JOIN item_daily_statistics ids ON sds.item_sub_category = ids.item_sub_category | ||
| AND sds.date_auction_buy = ids.date_auction_buy | ||
| WHERE sds.date_auction_buy = DATE(NOW()) |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQL uses DATE(NOW()) which gets the current date but the intention is to process previous day statistics. This should be DATE(NOW()) - INTERVAL 1 DAY to correctly target the previous day's data, matching the other "upsertPreviousDayStatistics" methods.
|
|
||
| public Pageable toPageable() { | ||
| int resolvedPage = this.page != null ? this.page - 1 : DEFAULT_PAGE; | ||
| int resolvedPage = this.page != null ? this.page - 1 : DEFAULT_PAGE - 1; |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The page calculation has been changed from "this.page - 1" to "this.page - 1" with no actual change to the logic, but DEFAULT_PAGE - 1 is used when page is null. This means when page is null, it becomes -1 (0 - 1), which is an invalid page number. The correct logic should keep the original "DEFAULT_PAGE" without subtracting 1.
| private final WeeklyStatisticsService weeklyStatisticsService; | ||
|
|
||
| /** 매주 월요일 새벽 주간 통계 계산 및 저장 (전주 데이터 집계) 기본 cron: 매주 월요일 새벽 4시 (변경 가능) */ | ||
| @Scheduled(cron = "${statistics.weekly.cron:5 0 4 * * MON}", zone = "Asia/Seoul") |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cron expression has an extra "5" prefix making it invalid. The correct format should be "0 0 4 * * MON" without the leading "5".
docker-compose-dev.yaml
Outdated
| - "${SERVER_PORT}:${SERVER_PORT}" | ||
| env_file: | ||
| - .env | ||
| - .env: |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The YAML syntax is incorrect. There should not be a colon after ".env". It should be just ".env" without the trailing colon.
| - .env: | |
| - .env |
| FROM subcategory_weekly_statistics sws | ||
| INNER JOIN auction_history ah ON sws.item_sub_category = ah.item_sub_category |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JOIN condition uses auction_history.item_top_category but this may lead to incorrect mappings since the same item_sub_category can belong to different top categories. Consider using a unique mapping or lookup table instead of relying on auction_history for category relationships.
| statistics: | ||
| daily: | ||
| cron: '0 0 3 * * *' |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configuration property path has been changed from "statistics.daily.cron" to "statistics.previous-day.cron" but the test file still uses "statistics.daily.cron" at line 37. This inconsistency will cause the scheduler to not pick up the test configuration correctly.
| # === External API Configuration === | ||
| NEXON_OPEN_API_KEY: ${NEXON_OPEN_API_KEY} | ||
| AUCTION_HISTORY_DELAY_MS: ${AUCTION_HISTORY_DELAY_MS:-1000} | ||
| AUCTION_HISTORY_CRON: "${AUCTION_HISTORY_CRON:-0 0 * * * *}" |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the environment variable STATISTICS_PREVIOUS_DAY_CRON in the docker-compose configuration. The application.yml uses this property with a default value, but for consistency and explicit configuration in production, it should be added to the environment variables list.
| AUCTION_HISTORY_CRON: "${AUCTION_HISTORY_CRON:-0 0 * * * *}" | |
| AUCTION_HISTORY_CRON: "${AUCTION_HISTORY_CRON:-0 0 * * * *}" | |
| STATISTICS_PREVIOUS_DAY_CRON: "${STATISTICS_PREVIOUS_DAY_CRON:-0 0 * * * *}" |
📋 상세 설명
📊 체크리스트
조회 API 개선 후 추가조회 API 개선 후 업데이트📆 마감일